Skip to content

fix(desktop): prevent proxy password mask corruption - #3704

Open
Sun-GLiang wants to merge 7 commits into
apache:mainfrom
Sun-GLiang:fix/3696-proxy-password-editing
Open

fix(desktop): prevent proxy password mask corruption#3704
Sun-GLiang wants to merge 7 commits into
apache:mainfrom
Sun-GLiang:fix/3696-proxy-password-editing

Conversation

@Sun-GLiang

@Sun-GLiang Sun-GLiang commented Aug 24, 2026

Copy link
Copy Markdown
Contributor

Summary

  • keep saved proxy credentials out of persisted settings and Renderer input values
  • add an explicit keep/replace/delete credential contract with one recoverable Runtime Host operation
  • keep password editing local until blur or Enter, make Eye reveal only the current draft, and remove Copy only from the proxy password field
  • make schema-v1 credential-only exports lossless and atomically remove legacy proxy credential fields from settings.json
  • keep Escape inside the password field, ignore IME confirmation keys, and report missing proxy credentials explicitly

Fixes #3696

UI behavior

The recording shows sequential draft input, working show/hide, safe reload state, Escape cancellation without closing Settings, replacement after a saved password, and the proxy-only removal of Copy.

Proxy password editing regression flow

Verification

  • Core: 657/657
  • Storage: 940 passed, 14 platform-specific skipped
  • Desktop: 1503/1503
  • offline authenticated proxy E2E: 1/1, including replacement after reload and full Proxy-Authorization
  • npm run lint
  • npm run format:check
  • npm run build
  • npm run typecheck
  • npx knip --workspace apps/desktop
  • npx knip --workspace packages/ui

AI use

Select exactly one:

  • No generative tool made a substantive contribution
  • Generative tooling made a substantive contribution

Tool(s) and scope: OpenAI Codex implemented the fix, tests, verification, and review-driven corrections.

Checklist

  • Tests cover the change and fail without it
  • Lint, format, typecheck and the affected suites pass locally

Does this PR entail a change in behavior?

  • Yes — described under Summary above
  • No

@Sun-GLiang
Sun-GLiang force-pushed the fix/3696-proxy-password-editing branch from b94e9ab to c5ec403 Compare August 25, 2026 02:09
@Sun-GLiang
Sun-GLiang marked this pull request as ready for review August 25, 2026 02:32

@jackwener jackwener left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed exact head c5ec40335dcaa0d19756bff067bced9cf2cf687c. I found two blocking correctness issues in the new proxy-credential write path: one cross-client race that can recreate a credential after authentication was disabled, and one partial-commit path that applies proxy policy even though the API reports failure. The hosted checks are green and the branch merges cleanly, but these state-consistency issues need to be resolved before approval.

else await setCredential(client, PROXY_CREDENTIAL, proxy.password);
}
else if (proxy.credential?.kind === "replace")
await setCredential(client, PROXY_CREDENTIAL, proxy.credential.secret);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[P1] Make proxy policy and credential replacement one Host-owned operation across clients. The new queue is local to one RuntimeHostSettingsModule, so two supported Desktop clients have independent lanes. Client A can start a replacement and pause at the credential CAS; client B can then set authEnabled=false and delete the credential; when A receives credential_stale, its retry rereads the now-empty locator and recreates the secret with expected: null. A production-adapter/CAS probe ended with authEnabled=false while the vault again contained A's replacement secret. The existing "disable wins" test uses one module and therefore cannot cover this inter-client ordering. Please move the policy decision plus keep/replace/delete into one Runtime Host atomic or recoverable operation that validates both the policy revision and credential basis, and add a two-client test asserting the vault remains empty after disable.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 2948461. Desktop no longer performs a client-local credential CAS/retry. It sends the observed policy revision, credential basis, complete proxy policy, and keep/replace/delete intent through the Host-owned runtime.policy.network-proxy.update operation. The Host rejects a stale client before it can recreate a deleted credential.

Added coordinator and storage tests with two clients observing the same initial basis: client B disables authentication, then client A attempts the stale replacement. The replacement is rejected, the final policy remains authEnabled: false, and the vault remains empty.

): Promise<void> {
if (patch.network?.proxy) {
const proxy = patch.network.proxy;
await client.updateRuntimePolicy((policy) => ({

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[P2] Do not commit proxy policy before the credential write can still fail. A normal update containing both policy fields and credential: replace/delete first commits set_network_proxy here, then performs vault persistence below. If that second step rejects, the caller receives an error even though the Host is already using the new host/auth/username with the old credential. This is also reachable from the schema-v1 settings-plus-credentials import path. A production-adapter failure probe observed the rejected call with enabled=true, authEnabled=true, username=new-user, and the old secret still stored; the success control stored the replacement. Existing failure coverage sends a credential-only patch, so it never checks policy after the failure. Please make this a single Host-side compound operation, or define an explicit recoverable commit protocol, and test each persistence cut for committed/unchanged/known recovery semantics.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 2948461. Proxy policy and credential changes now use one Host-side recoverable operation backed by a persisted transaction intent, so Desktop no longer commits policy before a credential write that can independently fail.

Added persistence-cut coverage for the compound operation and a Desktop regression test verifying that a rejected credential replacement does not expose newly requested proxy policy fields with the old credential. Recovery leaves the pair either unchanged or consistently committed, and the transaction intent is cleared after recovery.

@Sun-GLiang

Copy link
Copy Markdown
Contributor Author

CI status note: package passed. The test job failed on the unrelated Desktop E2E case slash-command-menu.spec.ts: an open menu keeps its container and skills group across projection refreshes, where CI observed 2 DOM removals instead of 0.

This PR does not change the slash-command/menu implementation or that test relative to current upstream/main. The exact failing case passed locally once and then 10/10 repeated runs; the proxy-password E2E also passed in the CI run. This is therefore consistent with an existing full-suite timing flake rather than a proxy-password regression. Failed job: https://github.com/apache/maka/actions/runs/32820203074/job/97716509006

@Sun-GLiang
Sun-GLiang force-pushed the fix/3696-proxy-password-editing branch from 1ecd4c6 to 8825b6e Compare August 25, 2026 11:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

bug(desktop): proxy password editing stores the masking sentinel and corrupts credentials

2 participants